Skip to content

Add support for Bitcoin Core 31.0#586

Open
xyzconstant wants to merge 5 commits into
rust-bitcoin:masterfrom
xyzconstant:add-support-for-core-31.0
Open

Add support for Bitcoin Core 31.0#586
xyzconstant wants to merge 5 commits into
rust-bitcoin:masterfrom
xyzconstant:add-support-for-core-31.0

Conversation

@xyzconstant
Copy link
Copy Markdown

@xyzconstant xyzconstant commented May 7, 2026

This PR adds support for downloading Bitcoin Core 31 and creates modules for v31 client types (mostly re-exports).

Changes from v30:

  • Drops settxfee types
  • Acknowledge new RPC commands: getmempoolcluster, getmempoolfeeratediagram, abortprivatebroadcast and getprivatebroadcastinfo in docs and verify.

NOTE: Updated and New RPCs in the release can be supported in further work, thus v31 client remains untested until then.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented May 7, 2026

Thanks for the contribution! Can you please explain how you went about this? Specifically exactly which parts did you use an LLM for? And if I do a review are you going to just feed the comments into an LLM or do the suggestions yourself?

Thanks

@xyzconstant
Copy link
Copy Markdown
Author

re: #586 (comment)

I used it for copying the modules from v30 (commit 2 was done mostly by the LLM with some tweaks from my side) and gather historical context about how major releases from Core are supported, #387 was my reference.

And if I do a review are you going to just feed the comments into an LLM or do the suggestions yourself?

Definitely myself :)

@xyzconstant xyzconstant force-pushed the add-support-for-core-31.0 branch from 6db6237 to 6022508 Compare May 7, 2026 14:18
Comment thread bitcoind/src/versions.rs Outdated
Comment on lines +91 to +92
not(feature = "31_0"),
not(feature = "30_2"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change confused me. Can you put a patch at the front of this PR that changes this to:

// This makes the build error more succinct if someone tries to build with --no-default-features.
#[cfg(not(feature = "0_17_2"))]
pub const VERSION: &str = "never-used";

And the reason this works is that later version features enable previous ones.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed e6bcc02 to address this

@tcharding
Copy link
Copy Markdown
Member

There is a slight problem here. In types a struct should only appear in the types/src/vX module if it is different from the type already defined. I.e. this PR adds v31::GetMempoolInfo but the v30::GetMempoolInfo is exactly the same.

Said another way, its unusual that there are no re-exports in v31/mod.rs from the v30 module.

Does that make sense? The types crate (and this whole repository) is slightly unusual. All re-exports and definitions are done in very particular places using very particular coding patterns. Holla if there is something you don't understand.

Thanks for the work!

@xyzconstant xyzconstant force-pushed the add-support-for-core-31.0 branch from 6022508 to 882b4e0 Compare May 9, 2026 05:25
@xyzconstant
Copy link
Copy Markdown
Author

re: #586 (comment)

Thanks for the review!

I've moved away from the copying approach and shifted to re-exporting from v30 as per your suggestion. Additionally, I've created f483441 to relax a check in verify that rejected re-exports-only versions.

@xyzconstant xyzconstant force-pushed the add-support-for-core-31.0 branch from 882b4e0 to bdfa22f Compare May 9, 2026 17:09
Copy link
Copy Markdown
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a look over and it looks good so far. I'll finish the check tomorrow.

How did you determine which RPCs needed updating and had to be marked as TODO in v31?

@xyzconstant
Copy link
Copy Markdown
Author

re: #586 (review)

Hi, thanks for reviewing the code!

How did you determine which RPCs needed updating and had to be marked as TODO in v31?

Actually there are more RPCs to be updated besides those I originally listed in the description (which I'll update). Initially I just read the release notes looking for removed/new RPCs and noticed a few with changes in the schema, then more surfaced while running integration tests.

@jamillambert
Copy link
Copy Markdown
Collaborator

jamillambert commented May 12, 2026

Actually there are more RPCs to be updated besides those I originally listed in the description ...

Can we then err on the side of caution and mark everything TODO unless the RPC has been checked and confirmed to be the same as in the version that v31 is reexporting.

For this PR it is fine to have a lot of TODOs and then in follow ups address them or remove them.

@xyzconstant
Copy link
Copy Markdown
Author

xyzconstant commented May 13, 2026

re: #586 (comment)

unless the RPC has been checked and confirmed to be the same as in the version that v31 is reexporting.

Hi, sorry, I don't get this at all. I thought integration tests already confirmed the re-exported RPCs are correct.

@jamillambert
Copy link
Copy Markdown
Collaborator

re: #586 (comment)

unless the RPC has been checked and confirmed to be the same as in the version that v31 is reexporting.

Hi, sorry, I don't get this at all. I thought integration tests already confirmed the re-exported RPCs are correct.

No they don't unfortunately. #58 explains it a bit further and what is being done to get closer to testing everything.

They catch a lot of cases with changes in the return shape of an RPC. But if there are new optional fields that are not returned it will not catch it. The docs of the more recent Core versions are fairly reliable and can be cross checked between versions to see if there are any changes.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented May 14, 2026

Please be warned @xyzconstant this is not a great repo for newer devs, I'm not saying you are one, just giving you a heads up. Also, unless you have some reason to want to continue contributing to this repo I would not think its worth your time to work out all the strangeness of the repo. But by all means don't let me stop you. Also please allow me to re-iterate I am extremely allergic to reviewing LLM code. Please make sure you understand the reason for every single line of code you push.

Based on https://bitcoincore.org/en/releases/31.0/ update docs by:
- Removing `settxfee` RPC command
- Add new RPC commands `getmempoolcluster`,
`getmempoolfeeratediagram`, `abortprivatebroadcast`, and `getprivatebroadcastinfo`.

For types only `settxfee` has been dropped, new/updated RPCs can be supported in
further work.
@xyzconstant xyzconstant force-pushed the add-support-for-core-31.0 branch from bdfa22f to 3fb287f Compare May 14, 2026 16:32
@xyzconstant
Copy link
Copy Markdown
Author

re: #586 (comment)

Thanks for this context, I wasn't aware of it. I followed your advice and marked everything as TODOs in the docs table, also reduced change surface by removing integration tests.

re: #586 (comment)

Also, unless you have some reason to want to continue contributing to this repo I would not think its worth your time to work out all the strangeness of the repo.

I have some downstream work that tests v31, that's why I'm pushing this effort. Initially I was just going to add support for downloading the binaries but noticed the client was needed too and expanded the scope more than needed.

Regarding the LLM-generated code: I share your concerns on that and understand reading AI slop can be frustrating. I let the LLM do the work I felt was mechanical and indeed carefully reviewed every single line before pushing since it's never my intention to waste anyone's time.

Just reduced the scope:

bdfa22f -> 3fb287f (compare)

Also, I updated the PR's description to reflect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants